Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterator over all the enabled options #278

Merged
merged 10 commits into from Apr 28, 2022
Merged

Conversation

arturoc
Copy link

@arturoc arturoc commented Apr 19, 2022

Adds a new function that returns an iterator over all the enabled flags.

This implementation takes into account combined flags like the ones described in: #204 (comment)

It uses an impl Iterator as return type because using an specific Iter type implies using const generics which are not supported in earlier versions of rust and adding a const to the trait in order to express the correct dimension of the array as can be seen in the last solution proposed in #204.

Superseeds #204

This adds a new function that returns an iterator over all the enabled flags.

This implementation takes into account combined flags like the ones described in: bitflags#204 (comment)

It uses an `impl Iterator` as return type because using an specific Iter type implies using const generics which are not supported in earlier versions of rust and adding a const to the trait in order to express the correct dimension of the array as can be seen in the last solution proposed in bitflags#204.

This superseeds bitflags#204
@KodrAus
Copy link
Member

KodrAus commented Apr 20, 2022

Ah this is an interesting approach 🤔 Thanks for exploring this @arturoc! I'd love to see some test cases that demonstrate how it works, and ideally we'd be able to replace our debug implementation with it (that might require an implementation that yielded &'static strs instead of flags though, but we could call that iter_names or something.

@arturoc
Copy link
Author

arturoc commented Apr 20, 2022

Ah yes I forgot to bring the tests from the previous implementation, just pushed them in a new commit

@arturoc
Copy link
Author

arturoc commented Apr 20, 2022

Note how in the first test the flags appear in the wrong order due to the optimization to not go through all the options in every iteration. Removing that would show the flags in the correct order but would be slower for bitflags with a lot of options. I can try to do some benchmarks to understand if it's worth

@arturoc
Copy link
Author

arturoc commented Apr 20, 2022

Just figured a better way to optimize the iterator method so it preserves the order of the flags and the options array is a const now.

struct Flags: u32 {
const ONE = 0b001;
const TWO = 0b010;
const THREE = 0b100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a cfg'd-out flag to this as well just to make sure we handle those? Something like:

#[cfg(windows)]
const FOUR_WIN = 0b1000;
#[cfg(unix)]
const FOUR_UNIX = 0b10000;

and then use those matching cfgs to check that we iterate over those flags when they're available, but don't when they're not?

@KodrAus
Copy link
Member

KodrAus commented Apr 23, 2022

Thanks for working on this @arturoc! It's a nice simple approach you've ended up with. I like it!

@arturoc
Copy link
Author

arturoc commented Apr 26, 2022

Thanks! have added a test for cfg

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! This looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants